fix(ssr): preserve pinned module transport after #3290 - #3332
Conversation
Project environment requests accepted optional HeadersInit input but constructed the request headers with object spread. A Headers instance would not materialize its entries there, and caller-supplied values could replace required credentials when plain objects were used. Clone the optional headers first, then set the required Authorization and Accept values last. Constraint: Exact-head review requires optional headers not to override required Authorization or Accept and Headers input to work correctly Rejected: Keep object spread and only filter known optional headers | HeadersInit includes iterable and Headers forms that object spread cannot represent safely Confidence: high Scope-risk: narrow Tested: npx --yes deno@2.7.7 test --no-check --allow-all src/server/project-env/fetcher.test.ts src/server/runtime-handler/project-resolution.test.ts src/proxy/handler.test.ts src/utils/header-identity.test.ts Tested: npx --yes deno@2.7.7 fmt --check src/server/project-env/fetcher.ts src/server/project-env/fetcher.test.ts src/server/runtime-handler/project-resolution.ts src/server/runtime-handler/project-resolution.test.ts Tested: npx --yes deno@2.7.7 lint src/server/project-env/fetcher.ts src/server/project-env/fetcher.test.ts src/server/runtime-handler/project-resolution.ts src/server/runtime-handler/project-resolution.test.ts Tested: npx --yes deno@2.7.7 check src/server/project-env/fetcher.ts src/server/project-env/fetcher.test.ts src/server/runtime-handler/project-resolution.ts src/server/runtime-handler/project-resolution.test.ts Tested: git diff --check
Static SSR imports for same-origin module URLs were still emitted as absolute HTTP URLs with query pins. Deno resolves those through its module loader, bypassing the local module server path that handles pinned child lookups. Canonicalize the same-origin SSR case to the same path-pinned transport used by browser and computed imports, while leaving foreign origins unchanged. Constraint: Dependency-pinned module URLs must stay bound to the request snapshot without requiring network fetches for same-origin children Rejected: Keep absolute same-origin SSR URLs with query pins | Deno module loading bypasses the local request mock and can escape the module-server handler Confidence: high Scope-risk: narrow Tested: npx --yes deno@2.7.7 test --no-check --allow-all src/server/project-env/fetcher.test.ts src/server/runtime-handler/project-resolution.test.ts src/proxy/handler.test.ts src/utils/header-identity.test.ts src/modules/server/module-server.test.ts src/server/services/rsc/endpoints/endpoint-router.test.ts src/transforms/import-rewriter/url-builder.test.ts src/transforms/import-rewriter/__tests__/hydration-parity.test.ts Tested: npx --yes deno@2.7.7 lint src/server/project-env/fetcher.ts src/server/project-env/fetcher.test.ts src/server/runtime-handler/project-resolution.ts src/server/runtime-handler/project-resolution.test.ts src/transforms/import-rewriter/url-builder.ts src/transforms/import-rewriter/url-builder.test.ts src/transforms/import-rewriter/__tests__/hydration-parity.test.ts src/modules/server/module-server.test.ts Tested: npx --yes deno@2.7.7 check src/server/project-env/fetcher.ts src/server/project-env/fetcher.test.ts src/server/runtime-handler/project-resolution.ts src/server/runtime-handler/project-resolution.test.ts src/transforms/import-rewriter/url-builder.ts src/transforms/import-rewriter/url-builder.test.ts src/transforms/import-rewriter/__tests__/hydration-parity.test.ts src/modules/server/module-server.test.ts
The SSR module path pinning change alters the client boot bundle. Committing the deterministic generated artifact keeps release builds and pre-push generation clean without broadening runtime behavior. Constraint: pre-push generation rewrites the checked-in RSC bundle from the clean committed source Rejected: Restore generated artifact without committing | leaves pre-push dirty and hides the source-artifact mismatch Confidence: high Scope-risk: narrow Tested: deno task generate Not-tested: full test suite in this commit step
Generic HTTP cache fetches need same-origin SSR imports to remain absolute URLs carrying query pinning, while module-server loaders need the pinned path transport for strict release lookup. Splitting the helper preserves both contracts and forwards local-project identity through the MDX loader path. Constraint: guarded HTTP fetch cannot consume root-relative module-server paths Rejected: Use pinned paths for every SSR caller | breaks generic HTTP cache fetch with Invalid URL Confidence: high Scope-risk: moderate Tested: DENO_TESTING=1 VF_DISABLE_LRU_INTERVAL=1 SSR_TRANSFORM_PER_PROJECT_LIMIT=0 REVALIDATION_PER_PROJECT_LIMIT=0 NODE_ENV=production LOG_FORMAT=text deno test --preload=src/schemas/_test-setup.ts --no-check --allow-all --unstable-worker-options --unstable-net src/server/project-env/fetcher.test.ts src/transforms/import-rewriter/url-builder.test.ts src/transforms/esm/http-cache.test.ts src/transforms/mdx/esm-module-loader/module-writer.test.ts src/transforms/import-rewriter/__tests__/hydration-parity.test.ts src/modules/server/module-server.test.ts Tested: git diff --check Tested: deno fmt --check Tested: deno lint Tested: deno task typecheck
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe change propagates ChangesMDX rendering and module resolution
Project environment request headers
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RenderContext
participant PageRenderer
participant LayoutOrchestrator
participant MDXRenderer
participant ESMLoader
RenderContext->>PageRenderer: Pass isLocalProject
RenderContext->>LayoutOrchestrator: Pass isLocalProject
PageRenderer->>MDXRenderer: Load page with options object
LayoutOrchestrator->>MDXRenderer: Load layout with options object
MDXRenderer->>ESMLoader: Forward local-project and pinning context
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This follow-up PR restores/extends SSR dependency-pinning transport behavior that was intended to land after #3290, and updates MDX + import rewriting paths to carry the correct identity and pinning signals through SSR module loading.
Changes:
- Adds an SSR “path key” pinning helper (
appendSameOriginSSRDependencyPinningPathKey) and updates SSR import rewriting/tests to prefer pinned/_vf_modules/_pins/...transport where required. - Plumbs
isLocalProjectthrough the MDX ESM loader context so local-only HTTP fallbacks can be gated correctly. - Makes project-environment fetch headers (
Authorization,Accept) authoritative over optional caller-provided headers, with a targeted test.
Verification (from PR description):
- Focused behavioral suite passed (10 tests, 263 steps).
- Format/lint/type checks passed; pre-push gate mostly green with one unrelated flaky test that passed in isolation.
- I did not run commands in this review environment; safest next step is to rerun the focused suites touching MDX/module loading and SSR module rewrite paths.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/transforms/mdx/index.ts | Adds isLocalProject parameter and forwards it into the MDX ESM loader context. |
| src/transforms/mdx/esm-module-loader/types.ts | Extends ESMLoaderContext with isLocalProject. |
| src/transforms/mdx/esm-module-loader/loader-helpers.ts | Propagates isLocalProject into VF module import processing context. |
| src/transforms/mdx/esm-module-loader/import-transformer.ts | Switches SSR same-origin pinning to use the new path-key helper. |
| src/transforms/mdx/esm-module-loader/module-writer.test.ts | Updates MDX module cache identity test to understand path-key pinning transport and passes isLocalProject. |
| src/transforms/import-rewriter/url-builder.ts | Implements appendSameOriginSSRDependencyPinningPathKey and adjusts SSR query pinning helper behavior. |
| src/transforms/import-rewriter/url-builder.test.ts | Adds coverage for SSR query pinning vs SSR path-key pinning behavior. |
| src/transforms/import-rewriter/core.ts | Updates SSR target rewrite behavior to use the SSR path-key helper. |
| src/transforms/import-rewriter/tests/hydration-parity.test.ts | Updates expectations to match SSR path-key pinned module specifiers. |
| src/transforms/esm/specifier-resolver.ts | Adds helper for cacheable specifier normalization (module-origin aware). |
| src/server/project-env/fetcher.ts | Ensures required env fetch headers override optional caller headers; exports internals for testing. |
| src/server/project-env/fetcher.test.ts | Adds regression test for authoritative env fetch headers behavior. |
| src/modules/server/module-server.test.ts | Updates SSR nested module fetch assertions to match new pinning expectations. |
| src/server/services/rsc/endpoints/rsc-bundles.generated.ts | Regenerates checked-in RSC client bundles from updated source. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Normalize protocol-relative and case-variant HTTP imports before dependency pinning and cache resolution so equivalent same-origin module URLs cannot bypass snapshot transport.
Thread the server-owned local-project signal through page, layout, and RSC MDX loading so pinned module paths can use the guarded local module-server fallback. Add caller-boundary regressions for each rendering path.
|
Final review fix pushed in 64237b2. Found and fixed a production blocker in the path-pinned MDX transport: Regression coverage now includes matching, mismatched, and malformed path pins. The module-writer test server mock was also tightened to enforce the real server contract of exactly one pin transport. Validation:
The only review thread is resolved/outdated. I did not approve or merge. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 34 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/server/services/rsc/orchestrators/render-handler.test.ts:291
- The test captures
isLocalProjectviaargs[11], which is brittle and easy to break if theloadModuleESMsignature changes (or if an argument is inserted). Prefer an explicit parameter list so the assertion stays aligned with the call contract.
mutableRenderer.loadModuleESM = ((...args: unknown[]) => {
observedIsLocalProject = args[11];
return Promise.resolve({ default: () => null });
}) as typeof mdxRenderer.loadModuleESM;
src/rendering/page-rendering.test.ts:204
- The test captures
isLocalProjectviaargs[11], which is brittle and can silently drift if theloadModuleESMparameter list changes. Use an explicit parameter list and readisLocalProjectdirectly.
let observedIsLocalProject: unknown;
mutableRenderer.loadModuleESM = ((...args: unknown[]) => {
observedIsLocalProject = args[11];
return Promise.resolve({ default: () => null });
}) as typeof mdxRenderer.loadModuleESM;
src/rendering/orchestrator/layout.test.ts:19
- The test reads
isLocalProjectvia a magic index (args[11]), which is brittle and obscures what is being asserted. Prefer an explicit parameter list so the capture stays correct if the signature changes.
let observedIsLocalProject: unknown;
mutableRenderer.loadModuleESM = ((...args: unknown[]) => {
observedIsLocalProject = args[11];
return Promise.resolve({ default: () => null });
}) as typeof mdxRenderer.loadModuleESM;
|
The remaining test-maintainability finding is fixed at exact head 5872f71 (parent 64237b2). All three fragile args[11] captures now use explicit named parameters, with assertions unchanged and no production behavior change. Exact-head changed-area coverage passes (16 tests / 401 steps), formatting, lint, typecheck, verify:quick, and diff checks pass. The conflict-free synthetic merge against main f56afcd passes the affected 3 tests / 16 steps plus verify:quick. Review threads: 0 unresolved. Confidence: 98%. Fresh hosted CI and merge-queue combined checks remain required. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 34 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/server/services/rsc/orchestrators/render-handler.test.ts:291
- This test hard-codes the
loadModuleESMargument index (args[11]) to observeisLocalProject, which is brittle if theloadModuleESMsignature changes. Prefer reading the last argument so the test tracks the contract without relying on a fixed positional index.
mutableRenderer.loadModuleESM = (
_compiledProgramCode,
_adapter,
src/rendering/orchestrator/layout.test.ts:19
- This test hard-codes the
loadModuleESMargument index (args[11]) to observeisLocalProject, which is brittle if theloadModuleESMsignature changes. Prefer reading the last argument so the test tracks the contract without relying on a fixed positional index.
mutableRenderer.loadModuleESM = (
_compiledProgramCode,
_adapter,
src/rendering/page-rendering.test.ts:204
- This test hard-codes the
loadModuleESMargument index (args[11]) to observeisLocalProject, which is brittle if theloadModuleESMsignature changes. Prefer reading the last argument so the test tracks the contract without relying on a fixed positional index.
mutableRenderer.loadModuleESM = (
_compiledProgramCode,
_adapter,
|
Merge confidence: 94% for exact head 5872f71.\n\nReasoning: the current head has no unresolved review threads and local exact-head verification passed for the SSR/module loader, render orchestration, project env fetcher, RSC orchestrator, import-rewriter, specifier resolver, and MDX module-fetcher/writer paths changed by this PR. Verification run on the exact head included git diff --check, deno fmt/lint/check over all changed TS/TSX files, generate:manifests:check, and 15 targeted tests / 342 steps with 0 failures. I did not rerun the broad network-backed http-cache suite locally because this environment has known SOCKS/network fixture failures there; hosted CI remains the required gate before merge. |
The SSR pinned-module follow-up was green at its own head but dirty after main advanced. Merge current main and regenerate the RSC bundle endpoint so GitHub can validate the reviewed changes at the current base. Constraint: GitHub marked the PR dirty after main advanced. Rejected: Rebase the follow-up branch | merge preserves reviewed PR history while clearing the base state. Confidence: high Scope-risk: moderate Directive: Regenerate src/server/services/rsc/endpoints/rsc-bundles.generated.ts after resolving runtime service changes. Tested: deno fmt --check changed SSR/MDX/RSC files Tested: deno lint changed SSR/MDX/RSC files Tested: deno check changed SSR/MDX/RSC files Tested: focused SSR/MDX/RSC/dev-server tests passed Not-tested: Full repository pre-push suite for this branch after the merge
|
Updated this PR onto current What changed:
Verification on the resolved merge:
I am waiting for hosted checks on this exact head before assigning merge confidence or queueing. |
The hosted coverage shard preloads schema setup before running the import-map preloader tests. That changes microtask timing enough for the capacity-race test to await an earlier load before resolving the newly admitted load, letting the admitted load hit its artificial timeout even though the admission contract already succeeded. Resolve the newly admitted load immediately after observing it so the test still proves capacity was not missed without depending on preload-specific scheduling. Constraint: Hosted coverage runs with src/schemas/_test-setup.ts preload and parallel shard settings. Rejected: Increase the artificial timeout | this would hide the timing dependency instead of removing it. Confidence: high Scope-risk: narrow Tested: npx --yes deno@2.7.7 test --preload=src/schemas/_test-setup.ts --no-check --parallel --allow-all --unstable-worker-options --unstable-net src/modules/import-map/preloader.test.ts Tested: npx --yes deno@2.7.7 test --no-check --allow-all src/rendering/page-rendering.test.ts Tested: npx --yes deno@2.7.7 fmt --check src/modules/import-map/preloader.test.ts Tested: npx --yes deno@2.7.7 lint src/modules/import-map/preloader.test.ts Not-tested: Full repository pre-push suite, skipped because the branch already hit unrelated local parallel-suite flakes while the exact hosted failure now passes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 40 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/transforms/README.md:45
resultis assigned but never used in this quick-start snippet, which can be confusing for readers. Consider either using the return value (e.g., showing how to consume the transformed ESM) or removing the binding and just awaiting the call.
const result = await transformToESM(code, {
filename: "component.tsx",
jsx: "react",
});
src/transforms/mdx/esm-module-loader/module-fetcher/index.ts:71
- These new dependency-pin errors are defined locally with
defineError(...). Veryfront’s error-handling pattern expects new slugs to live in the centralized error registry (so they are consistently exported, documented, and de-duplicated), then imported here. Consider moving this definition (and the companiondependency-pin-mismatch) tosrc/errors/error-registry/module.tsand importing the constants from#veryfront/errors.
|
Pushed Root cause: under the hosted coverage command with Fix: resolve the newly admitted third load immediately after Local verification:
This is not a merge-confidence declaration yet. Hosted checks are still running on this exact head. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 40 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/transforms/mdx/index.ts:75
loadModuleESMuses a default value (= {}) foroptionsOrAdapter. When callers use the legacy positional signature and passundefinedfor the adapter (e.g.loadModuleESM(code, undefined, projectId, ...)), the default parameter turns thatundefinedinto{}, socontext.adapterbecomes truthy and the loader skips adapter auto-detection. This will later crash when the loader calls adapter methods (e.g.loadImportMap(projectDir, adapter)).
Use an undefined default for optionsOrAdapter and keep the optionsOrAdapter ?? {} fallback only in the options-object branch.
|
Merge confidence for exact head Reasoning:
Residual risk is moderate-low because this PR touches SSR/MDX module transport and import resolution paths, but review comments are resolved, targeted regressions cover the changed behavior, and the full hosted matrix is green at the exact head. |
|
Merge confidence: 93% for exact head Reasoning:
Residual risk is moderate-low because the PR repairs SSR/pinned module transport behavior, which is cross-cutting but covered by the hosted matrix and review-thread closure. This exceeds the 90% threshold and applies only to the exact SHA above. |
Regenerated rsc-bundles.generated.ts from merged sources and unioned the module-writer test imports from both sides.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 40 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/server/project-env/fetcher.ts:287
projectEnvFetcherInternalsis exported as a test-only surface, but its name does not match the existing in-repo convention for test-only exports (typically__*ForTests, e.g.src/transforms/esm/http-cache.ts:267-270exports__injectCachesForTests). Using the established naming pattern makes it harder to accidentally depend on this from production code and keeps internal APIs visually distinct.
/**
* Test-only access to the privileged fetch helper. Never import this outside
* `fetcher.test.ts`.
*
* The header-authority regression it guards (authoritative `Authorization`/
* `Accept` must be set after merging optional caller headers) is unobservable
* through `fetchProjectEnvVars`: the public path only ever passes a benign
* `x-project-slug` header, so a reintroduced spread-order bug would not change
* the public function's behavior in a test.
*
* @internal
*/
export const projectEnvFetcherInternals = {
fetchEnvironmentVariables,
} as const;
Summary
PR #3290 merged at stale head
d37308797daca1018a5181c37a832a10a82d28cdwhile its review-fix push was still running. This follow-up carries the commits that never reached that PR's GitHub head, plus the fixes that hardened them afterwards. It does not reopen or duplicate the 109-file #3290 change.The branch contains 11 commits, grouped by theme:
SSR module pinning transport
c5da86bdfKeep same-origin SSR module imports on pinned paths — preserve fetchable same-origin SSR module URLs while providing pinned path transport to strict module-server loaders.aecfdcfdcKeep prebundled RSC scripts aligned with pinned module paths — regenerate the checked-in RSC bundles from the committed source.20283a9afSeparate SSR module pinning transports — split the module-server path transport (/_vf_modules/_pins/<key>/..., consumed by the strict MDX loader) from the query transport (?pins=...&ssr=true, consumed by generic guarded fetch).64237b2bffix(mdx): validate pinned module path transport — fail closed on mismatched or malformed pin segments before any adapter access (production blocker fix).33f6e7094test(modules): remove vacuous fetch assertions — after the transport split the server emits pinned child paths instead of prefetching same-origin children, so the old nested-fetch assertions asserted nothing; replaced with pinned-path emission assertions.Specifier canonicalization
fa31d3ba3fix(ssr): canonicalize HTTP module specifiers — normalize uppercaseHTTP://and protocol-relative forms so they route through the guarded cache instead of reaching the runtime's unguarded loader.53e7d2b7bfix(ssr): preserve nested protocol-relative URLs — resolve//host/pathimports found inside fetched modules against their parent URL, falling back to the module-server origin.Trusted identity and credential authority
fc2ed317bKeep project env fetch credentials authoritative — set the privilegedAuthorization/Acceptheaders after merging optional caller headers so caller input can never override the internal credential.a1227db01fix(mdx): preserve trusted local project identity — thread server-constructedisLocalProjectthrough the MDX loader path;false/absent selects the restrictive branch.5872f71ddtest(ssr): name local-project loader argument.Review fixes (post-review)
26350e548fix(ssr): harden protocol-relative resolution and address review findings —https:; only the resolution base's own host may keep a plaintext local-dev scheme.toCacheableHttpSpecifierpath-transport shim from the query-transport consumer.MDXRenderer.loadModuleESMnow takes an options object instead of 12 positional parameters.projectEnvFetcherInternalsdocumented as@internaltest-only (the header-authority regression is unobservable through the publicfetchProjectEnvVars); documented the intentional query/fragment asymmetry inunwrapDependencyPinningPath.Verification
deno 2.7.7format, lint, and type checks passed.git diff --checkpassed.Follow-ups
loadModuleESMoptions-object refactor flagged as follow-up material was completed in26350e548.Summary by CodeRabbit
Bug Fixes
Improvements
Tests